Skip to content

Conversation

@davidcole1340
Copy link
Collaborator

Zend strings now come in two forms - the borrowed &ZendStr and the owned ZendString, which can be compared to &str and String.

Replaced by returning references to the actual hashtable, and having a
new type `OwnedHashTable` when creating a new hashtable.
`&ZendStr` is now equivalent to `&str`, while `ZendString` is equivalent
to `String`.
@@ -1,6 +1,18 @@
<?php

<<<<<<< HEAD
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to rebase this properly, but I'm going to delete example/skel soon as it's just my testbench at this point

/// Panics if the function was unable to allocate memory for the Zend string.
pub fn from_c_str(str: &CStr, persistent: bool) -> Self {
let ptr = unsafe {
ext_php_rs_zend_string_init(str.as_ptr(), str.to_bytes().len() as _, persistent)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of producing a safe and correct wrapper around this, the persistent field probably should be hoisted into the type. It is not safe to provide request-arena allocated strings, for example, to a function that expects persisted ones - while the opposite is likely fine. We have covariance.

I don't think this can be easily expressed via lifetimes, but should be expressible with type variables:

struct ZendStringBase<Alloc>;

pub type ZendString = ZendStringBase<Request>;

pub mod persistent {
    pub type ZendString = ZendStringBase<Persistent>;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, the above should be perfectly fine so long as we're content with only using rust to extend PHP.

However, I'm not super familiar with how request allocated memory interacts with embedded PHP. I assume the lifetime of a request might be in the control of the hosting language.

If so, that opens the door to another safety issue: use after free after the request arena is deallocated. So another option might be to make ZendString::new always persistent and instead add some sort of token that represents the current request into the API - even if its largely an artificial abstraction.

This request object then must used to allocate per-request data and can be used to coordinate lifetimes

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The detail I'm still not sure about, I've been digging through php-src to figure this out, is where does the emalloc heap get cleared.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forcing all owned Zend strings to be persistent might be the best way I think, calling zend_string_release once their GC cycle is zero still causes them to be deallocated, however we lose the memory leak checks of the ZMM.

I'm not sure how we could account for the user potentially keeping a request-bound ZendString past the request boundary, leaving a dangling pointer inside of ZendString. Maybe just forcing all strings to be persistent would be the safest option?

Copy link
Collaborator

@vodik vodik Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how we could account for the user potentially keeping a request-bound ZendString past the request boundary, leaving a dangling pointer inside of ZendString

Explicitly creating a token type that wraps the request is probably the way to go: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c431c93f58529be80decb279e61589bc

In the example, even through TEST is static, if I grab it through fake_alloc I cannot get a borrow that outlives the Request token type.

Alt with a closure: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8254a995ae5213f44de52aef2eaa87cf

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we represent the lifetime of the request though? A Request object could either live for the lifetime of the extern "C" wrapper function or 'static (or maybe I am missing something)?

The ZendString API is technically an 'internal' API, so perhaps it could be deemed unsafe (or default to persistent with an unsafe request-bound option)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it seems that you aren't supposed to put persistent strings in zvals... When calling zend_ptr_dtor() (from the Drop impl or when called from PHP) it calls zend_string_destroy which only attempts to deallocate using the Zend memory manager.

@davidcole1340
Copy link
Collaborator Author

Going to merge this for now and can revisit later.

@davidcole1340 davidcole1340 merged commit aea8717 into master Sep 27, 2021
@davidcole1340 davidcole1340 deleted the zend-str branch September 27, 2021 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants